Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cache): improve caching and deletion of images #513

Merged
merged 2 commits into from
Mar 24, 2023
Merged

Conversation

dskuza
Copy link
Contributor

@dskuza dskuza commented Mar 24, 2023

Summary

Improve how images are (un)cached when there are Image changes in Source.

References

IN-1222

Implementation Details

The usage of ImagesController is now simplified - we will handle Image changes when all changes have been saved, rather than on each insert / delete / etc. There is now some additional logic for when images should be downloaded:

  1. Checked for any orphaned Images (Images that have no .item)
  2. Generate a set of all URLs for the latest Images
  3. Generate a set of all URLs for orphaned Images
  4. Create sets of URLs that should be downloaded, skipped, and deleted
  5. Delete the appropriate cached Images, download the appropriate uncached Images, and delete all orphan Images from source

Test Steps

  • Log in to Pocket
  • Let Home / Saves load, and let thumbnails load for some (or all) items
    • A spinner should appear while a thumbnail is downloading
  • Close and reopen the application
  • Let Home / Saves appear, and thumbnails should be immediately loaded if previously visible; no spinner should appear

PR Checklist:

  • Added Unit / UI tests
  • Self Review (review, clean up, documentation, run tests)
  • Basic Self QA

@dskuza dskuza requested a review from a team as a code owner March 24, 2023 18:35
@dskuza
Copy link
Contributor Author

dskuza commented Mar 24, 2023

The overall changelog of this pull request, while may look "large", is rather small; there are a bunch of renames, and then updated tests (which definitely affect the total changes).

case .move:
return
@unknown default:
func handle(images: [Image]?) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: can we name it other than "handle"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timc-mozilla probably, i couldn't think of a good name. do you have any suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handleImageCache?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timc-mozilla ¯_(ツ)_/¯ but the thought of renaming this is surprisingly overwhelming so i'll leave it for now 😆

// Delete all orphan URLs that are not to be skipped
let toDelete = orphanURLs.subtracting(skipDelete)

toDelete.forEach { delete(url: $0) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dskuza are these async methods? if not we should make them async

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bassrock not really… they "rely" on kingfisher code (behind the scenes and some protocols) which does things async, so there's not really anything synchronous here. the KF functions themselves aren't async, they're just performed async (using dispatch queues, i believe)

@dskuza dskuza merged commit a75e308 into develop Mar 24, 2023
@dskuza dskuza deleted the fix-image-cache branch March 24, 2023 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants